ADFA-5542: let the server say a build was cancelled by davidschachterADFA · Pull Request #1807 · appdevforall/CodeOnTheGo

davidschachterADFA · 2026-09-07T23:43:38Z

PR 14 on the carousel stack. Base is feature/ADFA-5554-longer-tooltip-hold (#1806); review that first.

Read the second commit, not the first. The first took an approach the review dismantled; the second replaces it and is what this PR is. Both are kept so the review that found the problem still reads against the code it was given.

The defect

EditorBuildEventListener held cancelRequested, a bare boolean meaning "a cancel happened recently". The only thing tying it to the build it belonged to was the order two main-thread runnables happened to run in — onBuildCancelRequested inline from the UI thread, prepareBuild posted from the build's thread. A cancel arriving between the post and its run was cleared by it, and the build the user stopped came back annotated BUILD_FAILED.

The fix: stop guessing, read the answer

The tooling server already knows, exactly and per build:

// ToolingApiServerImpl.kt
private fun getTaskFailureType(error: Throwable): Failure =
    when (error) {
        is BuildException -> BUILD_FAILED
        is BuildCancelledException -> BUILD_CANCELLED
        ...

It computed that, handed it to the caller of executeTasks, and notified the client with a BuildResult carrying tasks, an id and a duration — everything except the answer. BuildResult carries failure now, the same value from the same call, and the listener reads it.

#	Site	Change
F01	`BuildResult`	`+ val failure: TaskExecutionResult.Failure? = null`
F02	`ToolingApiServerImpl`	both failure sites classify the throwable once and use it for the `BuildResult` and the return value
F03	`EventListener.onBuildFailed`	`(tasks, failure)`
F04	`EditorBuildEventListener`	`outcomeKind(failure)` — `BUILD_CANCELLED` iff the server said so
F05	`EventListener`	`onBuildCancelRequested` removed, with its call in `cancelCurrentBuild`

What this deletes

The first commit's whole apparatus: runningBuildId, the @Volatile it needed and its non-atomic compare-and-clear, cancelledBuildId, forget(), the build id on the outcome callbacks. annotatedBuild goes back to a boolean cleared per build above the activity check — where it was, and where it belongs. Keying it to a build id existed only to avoid a reset, and the reset was never the problem; the cancel flag's reset was, and that flag is gone.

161 insertions against 191 deletions — net shorter than the code it replaces, fixing more.

onBuildCancelRequested is removed rather than left as a no-op: it existed solely so the listener could tell a cancel from a failure, and it is now shown that instead of being told it. That un-does one member ADFA-5509 (#1792) added, lower in this stack. The invariant that came with it — no defaulted members on EventListener, so the compiler asks every implementer — stays, and still guards the rest.

Tests

Test	Fails without the fix
`GIVEN a build the user stopped WHEN it fails THEN the client is told it was a cancel` (server)	yes — dropping `failure` from the notified `BuildResult` fails it, and nothing else. This is the regression test, on the server, because that is where the fix is
`a failure reaches the listener with the server's reason for it` (wrapper)	asserts the argument: a wrapper forwarding the call and dropping the reason would be the old defect one layer in, with no signature to complain
`every other reason the server gives is a failure`	the substance of the mapping — a dropped connection or an unsupported Gradle version must not read as the user stopping something
`a failure the server did not classify is a failure`	null must not become a cancel
`the server saying a build was cancelled is what marks it cancelled`	the positive case
`preparing a build clears a stale pairing, even with no activity attached`	restored; the clear sits above the activity check, so moving it below fails this

:app:testV8DebugUnitTest, :idetooltips:testV8DebugUnitTest, :subprojects:tooling-api-impl:test and spotlessCheck green.

A pre-existing break, fixed in passing

:subprojects:tooling-api-impl's test source has not compiled since ADFA-2784 added buildId to InitializeProjectParams — testInitParams never passed it. It is broken on stage too, and no workflow runs any :subprojects: tests, which is why nothing said so. One parameter here so this PR's server test can run; the CI gap wants a ticket of its own.

Review findings addressed

Every finding against the first commit is either fixed or no longer has anything to attach to: the Stop control racing prepareBuild, the --scan retry reusing a BuildId, the inert regression test, the untested forget(), the annotatedBuildId doc claim, the volatile read-modify-write, the dead @VisibleForTesting, the duplicated comparison, and the outcomeKind KDoc that described a Boolean.

The branch name still says cancel-by-build-id, from the approach that is gone. Left alone rather than churn an open PR's ref.

🤖 Generated with Claude Code

https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j

The listener held a bare flag meaning "a cancel happened recently", and the only thing tying it to the build it belonged to was the order two main-thread runnables happened to run in. onBuildCancelRequested is raised on the UI thread and so runs inline; prepareBuild is raised from the build's own thread and so is posted. A cancel landing after that post and before it ran was cleared by it, and the build the user stopped was annotated as a failure -- which is the one thing BUILD_CANCELLED exists to prevent. Fixing the order would only move the window, so the cancel is keyed to a build instead. BuildInfo and BuildResult already carry a BuildId; the listener interface was dropping it one line from where it was needed. onBuildCancelRequested, onBuildSuccessful and onBuildFailed now carry the id, the service remembers which build is running, and the listener compares rather than sequences. That subsumes the other flag too. annotatedBuild existed for the same missing information -- the outcome callbacks are handed the server's task list, not the one prepareBuild saw, so the listener kept a flag to know whether the finish it was looking at belonged to the start it drew. Both are now nullable ids, and neither needs clearing per build: an id left from a build whose outcome never arrived cannot match the next build's, so prepareBuild no longer resets anything. The interface has no defaulted members (ADFA-5509), so every widening is a compile error for both implementers rather than a silent drop; the wrapper test now also asserts the id survives the forward, since a wrapper that passed the call and dropped the argument would be that same defect one layer in with nothing to complain about it. The decision is extracted as outcomeKind so it can be asserted without a live activity, matching isAnnotated beside it. Restoring the clear prepareBuild used to do fails the new test, expected BUILD_CANCELLED but was BUILD_FAILED, and fails nothing else. Not reproduced on device: the Stop control only appears after a 150ms delayed menu invalidation queued behind prepareBuild, so the window is not reachable by hand. It is reachable in a test, which is what the regression case drives. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j

claude

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

…A-5542-cancel-by-build-id

Replaces the approach in the commit before this one, which had the client reconstruct whether a failure was a cancel by remembering which build was running and matching ids against it. The review found the premise wrong: the Stop control goes live at build submission, not when the server calls back prepareBuild, so the window was still open and now failed silently -- runningBuildId was null and the cancel was dropped by the very guard meant to protect it. It found the better answer too. The tooling server already knows. It catches the throwable Gradle raised and getTaskFailureType maps a BuildCancelledException to Failure.BUILD_CANCELLED -- one classification, no threading, no ambiguity -- and then hands that answer only to the caller of executeTasks while the BuildResult it notifies the client with carries tasks, an id and a duration. The question was answered exactly, one line from where it was needed, and thrown away. So BuildResult carries the failure now, the same value the return path gets, from one call. Both server failure sites classify once and use it twice. The listener reads it. That deletes the whole of the previous attempt: runningBuildId, the volatile it needed and its non-atomic compare-and-clear, cancelledBuildId, forget(), the build id on the outcome callbacks, and onBuildCancelRequested itself, which existed only to tell the listener something it can now be shown. annotatedBuild goes back to a boolean cleared per build, above the activity check, which is where it was before and where it belongs -- the reason to key it to a build was to avoid a reset that was never the problem. Net 30 lines shorter than the code it replaces, in a change that fixes more. ADFA-5509's abstractness invariant on EventListener stays and still guards every remaining member. The wrapper test that covered the removed callback now covers the failure reason instead, for the same reason it existed: a wrapper that forwarded the call and dropped the argument would be that defect one layer in, with no signature to complain. The regression test is on the server, because that is where the fix is: a build that fails with BuildCancelledException must report BUILD_CANCELLED to the client and not only to its caller. Dropping the field from the notified BuildResult fails it, and nothing else. Also fixes the compile of :subprojects:tooling-api-impl's tests, which have been broken since ADFA-2784 added buildId to InitializeProjectParams: testInitParams never passed it. It is broken on stage too. No workflow runs :subprojects: tests, which is why nothing said so -- worth a ticket of its own. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j

…A-5542-cancel-by-build-id

… on the chart Two findings from the second review. The sibling sweep was not done. The chart marker was fixed and the three reports beside it were left saying "failed", so a user who pressed Stop got a red "Build failed" bar, a "Build failed" notification in the shade, and an isSuccess=false result carrying failure text posted to every plugin listener -- their own deliberate action read back to them as an error in every place but one. The argument the PR gave for the marker applies verbatim to all four, and `failure` was already in hand at each. The notification, the bar and the message now follow it. The plugin API cannot express a cancel at all -- IdeServices.onBuildFailed takes an error string and nothing else -- so the message is the whole of what a plugin can be told, and that is now said in the code. The regression test drove initialize(), not executeTasks(), which is the path the ticket's defect actually travels: both sites got the same two-line edit, so deleting one left the suite green. Rather than stand up a live ProjectConnection to test the second site, the two are now one call. notifyBuildFailure classifies the throwable, notifies the client and returns the answer to its caller, so a site cannot report a failure without saying which, or tell the client one thing and its caller another. There is one place left to get this wrong, and the existing test covers it -- dropping the field still fails it and nothing else. The message is extracted as failureMessage for the same reason outcomeKind is: onBuildFailed returns early without an activity, so anything decided inside it cannot be reached from a test. The cancelled text is passed in rather than resolved, which is what makes it assertable. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j

davidschachterADFA · 2026-09-08T02:06:15Z

Second review round — both findings fixed

Pushed as 29e07894d.

The sibling sweep was not done, and the review was right to say so. The chart marker was fixed and the three reports beside it still said "failed": a red flashError bar, a "Build failed" notification in the shade, and an isSuccess=false result carrying failure text posted to every plugin listener. A user who pressed Stop had their own deliberate action read back to them as an error in every place but one. The argument this PR gave for the marker applies verbatim to all four, and failure was already in hand at each. Fixed at all four.

The plugin API genuinely cannot express a cancel — IdeServices.onBuildFailed takes an error string and nothing else — so the message is the whole of what a plugin can be told. That limit is now stated in the code rather than left as an omission.

The regression test drove the wrong path. It exercised initialize(), while the ticket's defect travels executeTasks(); both sites had the same two-line edit, so deleting one left the suite green. Rather than stand up a live ProjectConnection for the second, the two are now one call: notifyBuildFailure classifies the throwable, notifies the client and returns the answer to its caller. A site can no longer report a failure without saying which, or tell the client one thing and its caller another. That leaves one place to get this wrong, and the existing test covers it — dropping the field still fails it and nothing else.

On the Spotless mixing

Also flagged: c6d6393d5 carries a whole-file ktlint reformat of the server test inside a behavioural commit, which CLAUDE.md says to commit standalone. Correct, and I have not rewritten it — that commit is pushed and has been reviewed against, and re-writing it to tidy the diff would throw away the history the review reads against for no behavioural gain. Flagging it here instead so a reviewer knows which part of that 58-line diff is formatting: everything except the buildId fix and the new test.

The sibling sweep missed the path its own test drives. A sync the user stops arrives at ProjectHandlerActivity.postProjectInit as a failure carrying BUILD_CANCELLED, and the `when` there has no arm for it, so pressing Stop during a sync produced an indefinite red "Project initialization failed". The server-side regression test added last round exercises exactly this path, which makes "not reported as an error anywhere" false where it was most confidently claimed. The cancellation token outlived every build it belonged to. executeTasks cleared it on success and not on failure; the sync path never cleared it on any outcome at all, only shutdown() and an actual Stop did. So after any sync, and after any failed build, a token for a finished source sat in the server: the next Stop cancelled that dead source and answered wasEnqueued = true with nothing running -- and once a real build had started, left it running while telling the user it was being stopped. initialize(), which cancels first whenever a token is set, paid for a build that had ended long before. Both paths clear it in a finally now, and two tests fail without it. Removing onBuildCancelRequested took away the only request-time signal and nothing replaced the half that mattered. A Stop the server refuses -- NO_RUNNING_BUILD, or Gradle declining -- reached a log line at one call site and nothing whatsoever at the other: EditorPanelDockableContent threw the result away entirely. Both go through one reporter now, which says so. It also stops dereferencing failureReason with !!, which would have crashed on a refusal that carried no reason. Three more places still called a cancel a failure. BuildViewModel threw RuntimeException("Task execution failed: BUILD_CANCELLED") -- a cancel does not arrive as a CancellationException, so its catch could not tell, and the enum name was shown to the user as an error. The status line under the build output kept Gradle's own "BUILD FAILED", which onOutput copies there, contradicting the bar that had just said the build was stopped. And telemetry recorded success=false with no reason at all, so a deliberate cancel and a broken build were indistinguishable and every build-success rate counted the one the IDE makes easiest to press. The predicate is defined once now rather than derived three times in one callback, which is the duplicated-comparison finding from the previous round, reintroduced. The two KDocs contradicted each other about null. BuildResult.failure said null meant success; EventListener.onBuildFailed said it meant the server did not say. From this server it can be neither on a failure -- notifyBuildFailure classifies and returns non-null, and both catch paths go through it -- so the nullability exists for a server that does not classify. Both say that now, and the tests that cover null say they cover a defensive default rather than a reachable path. The test named "not reported as an error anywhere" asserted one of the four regressions it listed, so deleting the notification branch left it green. It is renamed to what it checks, and the notification and the telemetry reason now have tests of their own. Still unpinned, and said so in the test: the bar itself, and the cancelled-sync branch. Both need a live activity, which is what onBuildFailed returns early without. Not fixed, deliberately: info_build_cancelled exists in 3 of 14 locales and build_status_failed in 13, so ten locales trade a translated "Build failed" for an English "Build was cancelled by the user." I am not inventing translations. Every recently added string here is 3/14, so English fallback is this project's standing state rather than something this ticket introduced -- but the trade is real, and correct-in-English beats translated-and-wrong for a message that accuses the user of a failure they did not cause. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j

…A-5542-cancel-by-build-id

Merging stage through the stack broke this branch without a merge conflict. The cancelled-build arm added here calls finish(BuildState.Idle); stage has since moved that function onto reporter, so every other call site in the file arrived already saying reporter.finish and this one did not. Git had nothing to flag -- the two changes touch different lines -- and the branch simply stopped compiling. Fixed where the call was introduced rather than at the stack tip, so every branch above inherits it by merge instead of carrying its own copy. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j

davidschachterADFA · 2026-09-09T02:16:43Z

Superseded by #1812, which merges all of the carousel work onto current stage as a single change.

Closing rather than leaving open so review effort is not split: stage moved twice today (ADFA-5514 squash-merged, plus seven other commits), the stack's lower branches had diverged from it, and reviewing these individually meant reviewing against a base that no longer exists.

Nothing is lost. This branch is untouched, the commits and their messages are in #1812's history, and reopening is a click if that turns out to be the wrong call.

#1812 carries the ticket-by-ticket detail, the five conflict resolutions the ADFA-5514 squash forced — those compile either way, so they are the part worth reviewing hardest — and a device pass on a Pixel 6 Pro covering the daemon plot, the build markers and all three chart pages.

claude Bot reviewed Sep 7, 2026

View reviewed changes

davidschachterADFA and others added 2 commits September 7, 2026 17:24

Merge branch 'feature/ADFA-5554-longer-tooltip-hold' into feature/ADF…

e6123be

…A-5542-cancel-by-build-id

davidschachterADFA changed the title ~~ADFA-5542: say which build a cancel belongs to~~ ADFA-5542: let the server say a build was cancelled Sep 8, 2026

davidschachterADFA mentioned this pull request Sep 8, 2026

feat(metrics): annotate build start, finish and failure on the charts (ADFA-5509) #1792

Closed

davidschachterADFA and others added 3 commits September 7, 2026 18:33

Merge branch 'feature/ADFA-5554-longer-tooltip-hold' into feature/ADF…

8e3d0e7

…A-5542-cancel-by-build-id

Merge branch 'feature/ADFA-5554-longer-tooltip-hold' into feature/ADF…

042c1d3

…A-5542-cancel-by-build-id

This was referenced Sep 8, 2026

ADFA-5574: read each watched process the cheapest correct way #1808

Closed

ADFA-5487: Make the editor's memory chart a carousel of metric displays #1784

Closed

itsaky-adfa requested a review from a team September 8, 2026 11:46

davidschachterADFA and others added 6 commits September 8, 2026 11:16

Merge branch 'feature/ADFA-5554-longer-tooltip-hold' into feature/ADF…

e9c8144

…A-5542-cancel-by-build-id

Merge branch 'feature/ADFA-5554-longer-tooltip-hold' into feature/ADF…

800853b

…A-5542-cancel-by-build-id

Merge branch 'feature/ADFA-5554-longer-tooltip-hold' into feature/ADF…

27aa553

…A-5542-cancel-by-build-id

Merge branch 'feature/ADFA-5554-longer-tooltip-hold' into feature/ADF…

e37858e

…A-5542-cancel-by-build-id

davidschachterADFA closed this Sep 9, 2026

Navigation Menu

Uh oh!

Uh oh!

ADFA-5542: let the server say a build was cancelled - #1807

ADFA-5542: let the server say a build was cancelled#1807
davidschachterADFA wants to merge 12 commits into
feature/ADFA-5554-longer-tooltip-holdfrom
feature/ADFA-5542-cancel-by-build-id

davidschachterADFA commented Sep 7, 2026 •

edited

Loading

Uh oh!

claude Bot left a comment

Uh oh!

davidschachterADFA commented Sep 8, 2026

Uh oh!

davidschachterADFA commented Sep 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

davidschachterADFA commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The defect

The fix: stop guessing, read the answer

What this deletes

Tests

A pre-existing break, fixed in passing

Review findings addressed

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

davidschachterADFA commented Sep 8, 2026

Second review round — both findings fixed

On the Spotless mixing

Uh oh!

davidschachterADFA commented Sep 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

davidschachterADFA commented Sep 7, 2026 •

edited

Loading